Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate AR to Github Actions #8886

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Migrate AR to Github Actions #8886

merged 7 commits into from
Sep 25, 2023

Conversation

ioannakok
Copy link
Contributor

@ioannakok ioannakok commented Sep 21, 2023

Closes #8676

What does this change?

Why?

We are moving away from TC

Testing

Deployed to CODE and checked various types of articles:

  • Standard
  • Liveblog
  • Deadblog
  • Gallery

Build

Deployments

Build produced by GH Actions deployed successfully

Build produced by TeamCity and GH Actions produce the same artefacts

TeamCity GH Actions
image image

Zip produced by TC and GH Actions has the same structure

TeamCity GH Actions
image image

Release plan

ioannakok and others added 3 commits September 21, 2023 16:00
Co-authored-by: George B <705427+georgeblahblah@users.noreply.github.com>
Also, increase `buildNumberOffset` to 27000.

Co-authored-by: George B <705427+georgeblahblah@users.noreply.github.com>
Co-authored-by: George B <705427+georgeblahblah@users.noreply.github.com>
@ioannakok ioannakok force-pushed the migrate-ar-to-github-actions branch from 12a2586 to 5d2aad7 Compare September 21, 2023 15:01
@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

`apps-rendering/dist/assets/` contains `apps-rendering/dist/assets/manifest.json` and `apps-rendering/dist/assets/fonts` so no need to refer to those.
@ioannakok ioannakok changed the title Add ar-ci.sh script to run AR build in Github Actions Migrate AR to Github Actions Sep 22, 2023
Previous structure when unzipping was mobile-apps-rendering -> dist -> server -> <files>. This was breaking deployment. After this change structure will be the expected mobile-apps-rendering -> <files>

Co-authored-by: George B <705427+georgeblahblah@users.noreply.github.com>
yarn copy-manifest
yarn copy-fonts
yarn synth
zip -j dist/server/mobile-apps-rendering.zip dist/server/*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are replacing the deprecated node-riffraff-artifact with actions-riff-raff. node-riffraff-artifact was zipping under the hood the contents of dist/server and producing mobile-apps-rendering.zip. We can verify this in its config file. We are now adding a command to do the same.

The -j option in the command stores files with the given names only. The j option doesn't store directory information. This will produce structure:

mobile-apps-rendering
├── 713.server.js
├── manifest.json
├── server.js

which is the same as what current main produces.

image

@ioannakok ioannakok added the run_chromatic Runs chromatic when label is applied label Sep 22, 2023
@ioannakok ioannakok marked this pull request as ready for review September 22, 2023 15:56
@ioannakok ioannakok requested a review from a team as a code owner September 22, 2023 15:56
Copy link
Contributor

@DanielCliftonGuardian DanielCliftonGuardian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@georgeblahblah georgeblahblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great work @ioannakok! I had two suggestions which I think would be good to apply, but not necessarily required to proceed.

Comment on lines 29 to 31
- uses: actions/setup-node@v3
with:
node-version-file: .nvmrc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- uses: actions/setup-node@v3
with:
node-version-file: .nvmrc
- uses: actions/setup-node@v3
with:
cache: yarn
node-version-file: .nvmrc


- name: Install dependencies
run: |
npm i -g yarn@1.x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this step to install yarn separately!

Suggested change
npm i -g yarn@1.x

See more: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#about-caching-workflow-dependencies
Also, do not install yarn separately.

Co-authored-by: George B <705427+georgeblahblah@users.noreply.github.com>
Co-authored-by: DanielCliftonGuardian <110032454+DanielCliftonGuardian@users.noreply.github.com>
@ioannakok
Copy link
Contributor Author

ioannakok commented Sep 25, 2023

💎 Great work. A potential future optimisation is using actions cache https://github.com/actions/cache previously used here: https://github.com/guardian/apps-rendering/blob/ec2866f0345e8869de3e45b2de4ab11b2d3e20ec/.github/workflows/nodejs.yml#L18

Yes, sounds good @DanielCliftonGuardian, thanks for the recommendation! For this build I did it via the setup-node action. I will have a look to do similarly in the other workflows we did as part of this migration

@ioannakok
Copy link
Contributor Author

Restriction added

image

@ioannakok
Copy link
Contributor Author

Paused TC build

image

@ioannakok
Copy link
Contributor Author

ioannakok commented Sep 25, 2023

Replaced Apps Rendering Mono Repo (MAPI) with build which is the GH Action for the apps-rendering build. Will raise a follow-up PR to name this better

image

@ioannakok ioannakok merged commit 87a6b90 into main Sep 25, 2023
23 checks passed
@ioannakok ioannakok deleted the migrate-ar-to-github-actions branch September 25, 2023 11:50
@ioannakok
Copy link
Contributor Author

@ioannakok
Copy link
Contributor Author

Continuous Deployment kicked off and was successful: https://riffraff.gutools.co.uk/deployment/view/7d988a20-a9e8-4f80-ab0c-bd2e25fbd314

image

@ioannakok
Copy link
Contributor Author

Tried a few articles / liveblogs in apps-rendering PROD and they work fine

image

@ioannakok
Copy link
Contributor Author

Merged PR to improve naming #8904

georgeblahblah added a commit that referenced this pull request Sep 27, 2023
Migrating to GHA in #8886 has meant we are waiting on the AR status check to be
successful, but it doesn't always run because of the `paths-filter`.

This change removes the filter, so the action will always run.
georgeblahblah added a commit that referenced this pull request Sep 27, 2023
Migrating to GHA in #8886 has meant we are waiting on the AR status check to be
successful, but it doesn't always run because of the `paths-filter`.

This change removes the filter, so the action will always run.
georgeblahblah added a commit that referenced this pull request Sep 27, 2023
We migrated to GHA in #8886.

The repository has a status check on the AR build workflow, but the workflow doesn't always run because of the `paths-ignore`. This change removes the filter, so the action will always run and the status check will pass if the build is successful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run_chromatic Runs chromatic when label is applied
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate AR to GHA
3 participants